Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: deprecate ssh_skip_request_pty #222

Merged
merged 1 commit into from
Jul 29, 2024
Merged

chore: deprecate ssh_skip_request_pty #222

merged 1 commit into from
Jul 29, 2024

Conversation

tenthirtyam
Copy link
Collaborator

@tenthirtyam tenthirtyam commented Jul 8, 2024 •
edited
Loading

Description

Deprecate ssh_skip_request_pty per comment:

packer-plugin-vmware/builder/vmware/common/ssh_config.go

Lines 13 to 19 in 330dc55

type SSHConfig struct {
Comm communicator.Config `mapstructure:",squash"`
// These are deprecated, but we keep them around for BC
// TODO(@mitchellh): remove
SSHSkipRequestPty bool `mapstructure:"ssh_skip_request_pty"`
}

Testing

packer-plugin-vmware on  chore/remove-ssh_skip_request_pty [$!] via 🐹 v1.22.5 
⇣7% ➜ go fmt ./... 

packer-plugin-vmware on  chore/remove-ssh_skip_request_pty [$!] via 🐹 v1.22.5 
⇣7% ➜ make generate
2024/07/07 22:12:12 Copying "docs" to ".docs/"
2024/07/07 22:12:12 Replacing @include '...' calls in .docs/
Compiling MDX docs in '.docs' to Markdown in '.web-docs'...

packer-plugin-vmware on  chore/remove-ssh_skip_request_pty [$!] via 🐹 v1.22.5 
⇣7% ➜ make build   

packer-plugin-vmware on  chore/remove-ssh_skip_request_pty [$!] via 🐹 v1.22.5 took 3.2s 
⇣7% ➜ make test
?       github.com/hashicorp/packer-plugin-vmware       [no test files]
?       github.com/hashicorp/packer-plugin-vmware/version       [no test files]
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/common 6.603s
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/iso    2.159s
ok      github.com/hashicorp/packer-plugin-vmware/builder/vmware/vmx    1.716s

@tenthirtyam tenthirtyam added this to the v1.0.12 milestone Jul 8, 2024
@tenthirtyam tenthirtyam self-assigned this Jul 8, 2024
@tenthirtyam tenthirtyam requested a review from a team as a code owner July 8, 2024 02:15
@nywilken
Copy link
Contributor

nywilken commented Jul 17, 2024 •
edited
Loading

So this change requires a minor bump. When we remove configuration options that have been deprecated a minor is used to finalize the remove. Please let me know if you have any objections.

@nywilken nywilken added the version/bump minor A PR that changes behavior or contains breaking changes template configuration options. label Jul 17, 2024
nywilken
nywilken approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@nywilken nywilken left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I am approving but will hold on the merge until I get the okay since this needs to be released in a minor version.

@tenthirtyam
Copy link
Collaborator Author

Yep! I have this and the others all under the v1.1.0 milestone - we'll need to bump version.go.

@tenthirtyam tenthirtyam changed the title chore: remove ssh_skip_request_pty chore: deprecate ssh_skip_request_pty Jul 24, 2024
@tenthirtyam tenthirtyam force-pushed the chore/remove-ssh_skip_request_pty branch from f338284 to 3bdecaa Compare July 24, 2024 15:34
@tenthirtyam
Copy link
Collaborator Author

I've updated this to simply add a deprecation warning and that it will be removed in the next major. cc @nywilken @lbajolet-hashicorp

@tenthirtyam tenthirtyam force-pushed the chore/remove-ssh_skip_request_pty branch from 3bdecaa to 0ad2d6e Compare July 24, 2024 15:36
@tenthirtyam tenthirtyam added version/bump minor A PR that changes behavior or contains breaking changes template configuration options. and removed version/bump minor A PR that changes behavior or contains breaking changes template configuration options. labels Jul 24, 2024
lbajolet-hashicorp
lbajolet-hashicorp approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! I left a comment regarding the removal of c.Comm.SSHPty = false, just out of an abundance of precaution here, feel free to respond when you have time.

Pre-approving to not delay a future merge, since it's a minor concern, and whatever resolution will be good imo.

builder/vmware/common/ssh_config.go Show resolved Hide resolved
@tenthirtyam
chore: deprecated ssh_skip_request_pty
54ccaa3 
Deprecated `ssh_skip_request_pty` per comment:

```
// These are deprecated, but we keep them around for BC
// TODO(@mitchellh): remove"
```

Signed-off-by: Ryan Johnson <ryan@tenthirtyam.org>
@tenthirtyam tenthirtyam force-pushed the chore/remove-ssh_skip_request_pty branch from 0ad2d6e to 54ccaa3 Compare July 26, 2024 20:57
@lbajolet-hashicorp lbajolet-hashicorp merged commit 35732ba into main Jul 29, 2024
12 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the chore/remove-ssh_skip_request_pty branch July 29, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbajolet-hashicorp lbajolet-hashicorp lbajolet-hashicorp approved these changes

@nywilken nywilken Awaiting requested review from nywilken

Assignees

@tenthirtyam tenthirtyam

Labels
chore Chore tech-debt version/bump minor A PR that changes behavior or contains breaking changes template configuration options.
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tenthirtyam @nywilken @lbajolet-hashicorp